CLDSRV-750: add Server Access Logs #5980

leif-scality · 2025-10-22T10:07:23Z

No description provided.

bert-e · 2025-10-22T10:07:26Z

Hello leif-scality,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options

name	description	privileged	authored
`/after_pull_request`	Wait for the given pull request id to be merged before continuing with the current one.
`/bypass_author_approval`	Bypass the pull request author's approval	⭐
`/bypass_build_status`	Bypass the build and test status	⭐
`/bypass_commit_size`	Bypass the check on the size of the changeset `TBA`	⭐
`/bypass_incompatible_branch`	Bypass the check on the source branch prefix	⭐
`/bypass_jira_check`	Bypass the Jira issue check	⭐
`/bypass_peer_approval`	Bypass the pull request peers' approval	⭐
`/bypass_leader_approval`	Bypass the pull request leaders' approval	⭐
`/approve`	Instruct Bert-E that the author has approved the pull request.		✍️
`/create_pull_requests`	Allow the creation of integration pull requests.
`/create_integration_branches`	Allow the creation of integration branches.
`/no_octopus`	Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
`/unanimity`	Change review acceptance criteria from `one reviewer at least` to `all reviewers`
`/wait`	Instruct Bert-E not to run until further notice.

Available commands

name	description	privileged
`/help`	Print Bert-E's manual in the pull request.
`/status`	Print Bert-E's current status in the pull request `TBA`
`/clear`	Remove all comments from Bert-E from the history `TBA`
`/retry`	Re-start a fresh build `TBA`
`/build`	Re-start a fresh build `TBA`
`/force_reset`	Delete integration branches & pull requests, and restart merge process from the beginning.
`/reset`	Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

bert-e · 2025-10-22T10:07:32Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-750 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.1.5

Please check the Fix Version/s of CLDSRV-750, or the target
branch of this pull request.

codecov · 2025-10-22T10:21:42Z

Codecov Report

❌ Patch coverage is 91.66667% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.28%. Comparing base (fad11cb) to head (f5edc5a).
⚠️ Report is 5 commits behind head on development/9.2.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
lib/utilities/serverAccessLogger.js	89.65%	9 Missing ⚠️
lib/api/bucketPutCors.js	84.21%	3 Missing ⚠️
lib/api/bucketPutWebsite.js	90.47%	2 Missing ⚠️
lib/metadata/metadataUtils.js	92.59%	2 Missing ⚠️
lib/api/bucketDeleteCors.js	93.33%	1 Missing ⚠️
lib/api/bucketDeleteWebsite.js	92.85%	1 Missing ⚠️
lib/api/bucketGetCors.js	92.30%	1 Missing ⚠️
lib/api/bucketGetLogging.js	75.00%	1 Missing ⚠️
lib/api/bucketGetWebsite.js	92.85%	1 Missing ⚠️
lib/api/bucketPutLogging.js	66.66%	1 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
lib/Config.js	`79.86% <100.00%> (+0.27%)`	⬆️
lib/api/api.js	`91.62% <100.00%> (+0.40%)`	⬆️
lib/api/bucketGetLocation.js	`96.00% <100.00%> (+11.15%)`	⬆️
lib/api/multipartDelete.js	`100.00% <100.00%> (ø)`
lib/api/objectDelete.js	`94.06% <100.00%> (+0.10%)`	⬆️
lib/server.js	`78.37% <100.00%> (+1.10%)`	⬆️
lib/api/bucketDeleteCors.js	`93.54% <93.33%> (-1.33%)`	⬇️
lib/api/bucketDeleteWebsite.js	`93.54% <92.85%> (-1.33%)`	⬇️
lib/api/bucketGetCors.js	`96.42% <92.30%> (+13.09%)`	⬆️
lib/api/bucketGetLogging.js	`96.87% <75.00%> (-3.13%)`	⬇️
... and 6 more

@@                 Coverage Diff                 @@
##           development/9.2    #5980      +/-   ##
===================================================
+ Coverage            84.10%   84.28%   +0.18%     
===================================================
  Files                  193      194       +1     
  Lines                12335    12425      +90     
===================================================
+ Hits                 10374    10473      +99     
+ Misses                1961     1952       -9

Flag	Coverage Δ
ceph-backend-test	`65.08% <87.50%> (+0.30%)`	⬆️
kmip-ft-tests	`27.79% <53.40%> (+0.65%)`	⬆️
mongo-v0-ft-tests	`68.55% <87.12%> (+0.23%)`	⬆️
mongo-v1-ft-tests	`64.14% <86.74%> (-4.18%)`	⬇️
multiple-backend	`34.97% <57.57%> (+0.65%)`	⬆️
sur-tests	`35.31% <56.06%> (-0.20%)`	⬇️
sur-tests-inflights	`37.21% <56.06%> (+0.60%)`	⬆️
unit	`69.27% <56.43%> (-0.26%)`	⬇️
utapi-v2-tests	`34.10% <55.68%> (+0.65%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

bert-e · 2025-11-06T15:14:55Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-750 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.1.6

Please check the Fix Version/s of CLDSRV-750, or the target
branch of this pull request.

francoisferrand · 2025-11-07T15:53:11Z

@nicolas2bert @leif-scality Is this supposed to land in a patch release on 9.1 ?

nicolas2bert · 2025-11-10T10:18:05Z

lib/Config.js


+function parseServerAccessLogs(config) {
+    const res = {
+        enabled: true,


Should it be false by default?

Yes, let's set it to false by default so that access.log is not created in Artesca.

nicolas2bert · 2025-11-10T10:21:32Z

config.json

        "objectPutRetention": true
+    },
+    "serverAccessLogs": {
+        "enabled": true,


Can we also make sure the API relies on this toggle and returns Not Implemented if enabled: false?

bert-e · 2025-11-10T10:21:45Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-750 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.1.6
9.2.0

Please check the Fix Version/s of CLDSRV-750, or the target
branch of this pull request.

dvasilas

(partial review, but I have some comments that can be addressed)

What about tests?
I think we should have tests that verify that what gets written to server-access.log is what is expected, for every API.
It is ok for me to add tests separately in another PR if you prefer, we just need to create the ticket so that we don't forget.

dvasilas · 2025-11-11T16:03:04Z

lib/Config.js

+function parseServerAccessLogs(config) {
+    const res = {
+        enabled: true,
+        outputFile: './logs/server-access.log',


Suggested change

outputFile: './logs/server-access.log',

outputFile: '/logs/server-access.log',

dvasilas · 2025-11-11T16:08:04Z

lib/Config.js


+function parseServerAccessLogs(config) {
+    const res = {
+        enabled: true,


Yes, let's set it to false by default so that access.log is not created in Artesca.

lib/server.js

config.json

bert-e · 2025-11-17T09:04:21Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-750 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.2.0

Please check the Fix Version/s of CLDSRV-750, or the target
branch of this pull request.

dvasilas · 2025-11-17T09:24:29Z

lib/utilities/serverAccessLogger.js

Typo in the name of the file, there are 3 s.

dvasilas · 2025-11-17T09:27:31Z

lib/utilities/serverAccesssLogger.js

+            request.headers['cf-connecting-ip']; // Cloudflare
+
+        // x-forwarded-for can contain multiple IPs, take the first one
+        if (headerRemoteIP && headerRemoteIP.includes(',')) {


What if there is only 1 IP and there is no , ?
What about something like

if (headerRemoteIP) { remoteIP = headerRemoteIP.includes(',') ? headerRemoteIP.split(',')[0].trim() : headerRemoteIP; }

dvasilas · 2025-11-17T09:35:34Z

lib/api/multipartDelete.js

+                if (request.serverAccessLogs) {
+                    // eslint-disable-next-line no-param-reassign
+                    request.serverAccessLogs.analyticsBytesDeleted = partSizeSum;


Suggested change

if (request.serverAccessLogs) {

// eslint-disable-next-line no-param-reassign

request.serverAccessLogs.analyticsBytesDeleted = partSizeSum;

if (request.serverAccessLog) {

// eslint-disable-next-line no-param-reassign

request.serverAccessLog.analyticsBytesDeleted = partSizeSum;

dvasilas · 2025-11-17T09:36:08Z

lib/api/objectDelete.js

+                    if (request.serverAccessLogs) {
+                        // eslint-disable-next-line no-param-reassign
+                        request.serverAccessLogs.analyticsBytesDeleted = objMD['content-length'];


Suggested change

if (request.serverAccessLogs) {

// eslint-disable-next-line no-param-reassign

request.serverAccessLogs.analyticsBytesDeleted = objMD['content-length'];

if (request.serverAccessLog) {

// eslint-disable-next-line no-param-reassign

request.serverAccessLog.analyticsBytesDeleted = objMD['content-length'];

dvasilas · 2025-11-17T09:46:45Z

lib/Config.js

+
+    if (config && config.serverAccessLogs) {
+        if ('enabled' in config.serverAccessLogs) {
+            assert(typeof config.serverAccessLogs.enabled === 'boolean');


Suggested change

assert(typeof config.serverAccessLogs.enabled === 'boolean');

assert(typeof config.serverAccessLogs.enabled === 'boolean', 'bad config: serverAccessLogs.enabled not boolean');

dvasilas · 2025-11-17T09:52:42Z

package.json

    "@azure/storage-blob": "^12.28.0",
    "@hapi/joi": "^17.1.1",
-    "arsenal": "git+https://github.com/scality/Arsenal#8.2.37",
+    "arsenal": "git+https://github.com/scality/Arsenal#improvement/ARSN-531-export-server-access-log-fields",


Reminder to fix before merging.

dvasilas · 2025-11-17T10:09:57Z

lib/server.js

+            res.on('finish', () => {
+                // eslint-disable-next-line no-param-reassign
+                req.serverAccessLog.endTime = process.hrtime.bigint();
+                logServerAccess(req.serverAccessLog, req, res);


The first argument is a bit redundant, you could change the function to logServerAccess(req, res).

dvasilas · 2025-11-17T10:22:40Z

lib/utilities/serverAccessLogger.js

+        action: params.analyticsAction || null,
+        accountName: params.analyticsAccountName || null,
+        accountDisplayName: authInfo ? authInfo.getAccountDisplayName() : null,
+        userName: params.analyticsUserName || null,
+        clientPort: req.socket.remotePort || null,
+        httpMethod: req.method || null,
+        bytesDeleted: params.analyticsBytesDeleted || null,


Not sure if adding the analytics prefix to some fields is useful.
The situation in which it would make sense to have it is if we had a field with the same name and we wanted to distinguish between analytics and server access (analyticsAction and serverAccessAction).
Do you think we can remove it?

dvasilas · 2025-11-17T10:31:06Z

lib/utilities/serverAccesssLogger.js

+    const bytesSent = res.serverAccessLog.bytesSent;
+    const authInfo = params.authInfo;
+
+    serverAccessLogger.info('SERVER_ACCESS_LOG', {


I think we don't use this message; we could send an empty message instead to reduce a bit the size of each log

Suggested change

serverAccessLogger.info('SERVER_ACCESS_LOG', {

serverAccessLogger.info('', {

dvasilas · 2025-11-17T10:54:09Z

lib/metadata/metadataUtils.js

    const { bucketName } = params;
-    return metadata.getBucket(bucketName, log, (err, bucket) => {
-        storeServerAccessLogInfo(params.request, params.authInfo, bucket);
+    return metadata.getBucket(bucketName, log, (err, bucket, raftSessionId) => { // Extract raft session id


Suggested change

return metadata.getBucket(bucketName, log, (err, bucket, raftSessionId) => { // Extract raft session id

return metadata.getBucket(bucketName, log, (err, bucket, raftSessionId) => {

bert-e · 2025-11-17T11:08:20Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-750 contains:

9.2.1

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.2.0

Please check the Fix Version/s of CLDSRV-750, or the target
branch of this pull request.

bert-e · 2025-11-17T16:27:00Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

leif-scality force-pushed the improvement/CLDSRV-750-write-server-access-logs branch from 2e9e717 to 9404507 Compare October 22, 2025 10:09

leif-scality force-pushed the improvement/CLDSRV-750-write-server-access-logs branch 3 times, most recently from 00832f8 to a63c29f Compare October 23, 2025 20:52

leif-scality force-pushed the improvement/CLDSRV-750-write-server-access-logs branch from ed7705b to b95e090 Compare November 4, 2025 14:40

leif-scality force-pushed the improvement/CLDSRV-750-write-server-access-logs branch from 67a4483 to 9ecbb31 Compare November 6, 2025 15:22

leif-scality marked this pull request as ready for review November 7, 2025 15:45

leif-scality force-pushed the improvement/CLDSRV-750-write-server-access-logs branch from 6c4b7ec to 5cf2562 Compare November 7, 2025 15:48

nicolas2bert reviewed Nov 10, 2025

View reviewed changes

dvasilas reviewed Nov 11, 2025

View reviewed changes

dvasilas reviewed Nov 13, 2025

View reviewed changes

config.json Show resolved Hide resolved

leif-scality force-pushed the improvement/CLDSRV-750-write-server-access-logs branch 3 times, most recently from 81eb964 to 940992a Compare November 14, 2025 14:26

dvasilas changed the base branch from development/9.1 to development/9.2 November 17, 2025 09:04

dvasilas reviewed Nov 17, 2025

View reviewed changes

leif-scality added 6 commits November 17, 2025 17:26

CLDSRV-750: add Server Access Logs

5a73401

CLDSRV-750: replace repeated logic with standardMetadataValidateBucket

6e40876

CLDSRV-750: add raftSessionID to server access logs

8a483d8

CLDSRV-750: add analytics fields to server access logs

bcbf8ba

CLDSRV-750: skip server access logs for heatbeat and backbeat routes

614eb8b

CLDSRV-750: disable Put/GetBucketLogging if it is disabled in the config

aa15a14

leif-scality force-pushed the improvement/CLDSRV-750-write-server-access-logs branch from 940992a to f85dc8f Compare November 17, 2025 16:26

tmp arsenal branch

98b1872

leif-scality force-pushed the improvement/CLDSRV-750-write-server-access-logs branch 3 times, most recently from 71fbf80 to 9e2c0de Compare November 17, 2025 17:19

CLDSRV-750: fix startTime and elapsed_ms

f5edc5a

leif-scality force-pushed the improvement/CLDSRV-750-write-server-access-logs branch from 9e2c0de to f5edc5a Compare November 17, 2025 17:20

	outputFile: './logs/server-access.log',
	outputFile: '/logs/server-access.log',

	assert(typeof config.serverAccessLogs.enabled === 'boolean');
	assert(typeof config.serverAccessLogs.enabled === 'boolean', 'bad config: serverAccessLogs.enabled not boolean');

	serverAccessLogger.info('SERVER_ACCESS_LOG', {
	serverAccessLogger.info('', {

	return metadata.getBucket(bucketName, log, (err, bucket, raftSessionId) => { // Extract raft session id
	return metadata.getBucket(bucketName, log, (err, bucket, raftSessionId) => {

CLDSRV-750: add Server Access Logs #5980

Are you sure you want to change the base?

CLDSRV-750: add Server Access Logs #5980

Conversation

leif-scality commented Oct 22, 2025

Uh oh!

bert-e commented Oct 22, 2025

Hello leif-scality,

Uh oh!

bert-e commented Oct 22, 2025

Incorrect fix version

Uh oh!

codecov bot commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bert-e commented Nov 6, 2025

Incorrect fix version

Uh oh!

francoisferrand commented Nov 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bert-e commented Nov 10, 2025

Incorrect fix version

Uh oh!

dvasilas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bert-e commented Nov 17, 2025

Incorrect fix version

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 22, 2025 •

edited

Loading